Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fix write stream reconnection issue #180

Merged
merged 4 commits into from
Sep 5, 2024

Conversation

mattisonchao
Copy link
Member

@mattisonchao mattisonchao commented Sep 5, 2024

Motivation

the current implementation didn't replace the failed stream to a new stream. that causes client constantly get exception when write data to server due to oxia server restart.

Modification

  • Replace the invalid stream with a new one.

Others

I am considering the performance affection here, let us try improve in the future.

addExposedPorts(OXIA_PORT, METRICS_PORT);
if (fixedServicePort) {
int freePort = findFreePort();
addFixedExposedPort(freePort, OXIA_PORT);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why exposing a fixed port? There is already a way to get the port with random number and use it outside the container (eg: getMappedPort(OXIA_PORT))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because I wanna testing the client behaviour when server restart. existing mechanism will use different ports after restart the container. that will make the client can't reonnect to the server.

@merlimat merlimat merged commit 0e43403 into main Sep 5, 2024
1 check passed
@merlimat merlimat deleted the fix.write.stream.reconnection branch September 5, 2024 22:42
mattisonchao pushed a commit to streamnative/oxia that referenced this pull request Sep 20, 2024
Get rid of failed write-streams in Go client SDK. 

This is similar change to
streamnative/oxia-java#180 in Java SDK.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants